Support SetsRequiredMembersAttribute #60392

333fred · 2022-03-25T19:33:01Z

The SetsRequiredMembersAttribute prevents the compiler from checking the required member list of a type when calling that constructor, and suppresses any errors from a base type's list being invalid.

Specification: https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md
Test plan: #57046

333fred · 2022-03-25T19:33:20Z

@RikkiGibson @jcouv please take a look.

The SetsRequiredMembersAttribute prevents the compiler from checking the required member list of a type when calling that constructor, and suppresses any errors from a base type's list being invalid. Specification: https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md Test plan: dotnet#57046

333fred · 2022-03-28T21:24:08Z

@jcouv @RikkiGibson please take a look

RikkiGibson

Just had a few comments so far. Haven't looked at the tests yet--possible those will hold answers to some of the questions anyway. Will review the tests a little later this afternoon.

src/Compilers/CSharp/Portable/Symbols/Source/SourceConstructorSymbolBase.cs

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

src/Compilers/CSharp/Portable/Symbols/SymbolExtensions.cs

333fred · 2022-03-29T22:20:08Z

@RikkiGibson @jcouv please take a look.

RikkiGibson

LGTM aside from some minor comments.

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs

333fred · 2022-03-29T23:36:18Z

@jcouv for the second review please.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

jcouv

Done with review pass (iteration 7)

jcouv · 2022-03-30T19:51:25Z

Just curious, what was the problem you ran into that the used assemblies leg detected?

In reply to: 1083559918

333fred · 2022-03-30T20:18:03Z

Just curious, what was the problem you ran into that the used assemblies leg detected?

d9da29b. It wasn't happy about mismatched collection of dependencies.

…operties should be considered initialized.

333fred · 2022-04-06T00:21:38Z

@RikkiGibson @jcouv please take another look.

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

RikkiGibson · 2022-04-07T19:45:33Z

I was contemplating whether it's reasonable for constructors to delegate to 'SetRequiredMembers' constructors, without being 'SetsRequiredMembers' themselves. For example, test RequiredMemberSuppressesNullabilityWarnings_ChainedConstructor_08.

class Base
{
    public required string Prop1 { get; set; }

    [SetsRequiredMembers]
    public Base() { Prop1 = "a"; }
    
    public Base(int unused) { }
}

class Derived : Base
{
    public required string Prop2 { get; set; }

    // maybe compiler needs to emit [SetsRequiredMembersOf(typeof(Base))] here?
    public Derived() : base() { }

    // maybe compiler needs to emit [SetsRequiredMembersOf(typeof(Base))] here?
    public Derived(int unused) : this() { }
}

class Program
{
    static void Main()
    {
        // how do we realize the user doesn't need to set Prop1?
        var derived = new Derived() { Prop2 = "a" };
        
        // we seem even more at sea here :)
        var derived2 = new Derived(1) { Prop2 = "a" };
    }
}

The metadata encoding spec doesn't appear to be able to represent this as written. Did I miss something?

I could imagine complex type hierarchies peppering in 'SetsRequiredMembers' at arbitrary levels. It feels like representing that accurately would require tracking the most-derived type in the hierarchy which has the attribute. When the compiler binds a constructor it can determine which base constructor it ultimately delegates to, perhaps using a similar mechanism as is currently used to prevent cycles in constructor initializers.

However, it feels to me like it would be a very reasonable starting place to require any constructors which delegate to a 'SetsRequiredMembers' constructor to also have 'SetsRequiredMembers'. This would make it an all-or-nothing thing down the hierarchy to start with, and I think it leaves the space open for us to add something like what I described above later on.

333fred · 2022-04-12T20:10:29Z

@RikkiGibson we could do that. My initial thought was, however, not to do any form of restrictions. In short, in your example, there is no way to know that Prop2 should not be set from the derived constructors. I'm ok with considering a simplification of this, however, as that would significantly reduce the complexity of the changes in NullableWalker here.

…o setsrequiredmembers * upstream/features/required-members: (808 commits) Update for definite assignment changes Remove duplicate package references (dotnet#60658) Formatting and code generation options (dotnet#60127) Trim unnessasary leading lines when removing usings (dotnet#60672) Pass options to FixAllAsync, simplify CodeAction registration (dotnet#60665) Fallout Lint Restore CodeStyle test projects Update struct field definite assignment tests Global indentation options - take 2 (dotnet#60565) Run continuation to dispose of cancellation token source (dotnet#60653) Fixup Update tests Cleanup Cleanup move properties Delay starting the work to scan for todo-items Simplify Clean up syntax context Clean up syntax context ...

…n chaining to a constructor with the attribute. Add more tests.

333fred · 2022-04-14T01:14:39Z

@RikkiGibson @jcouv please take another look.

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs

jcouv

LGTM Thanks (iteration 12)

…o setsrequiredmembers * upstream/features/required-members: (66 commits) Fix dotnet#55183: Add SymbolVisitor<TArgument, TResult> (dotnet#56530) Simplifier options (dotnet#60174) Remove duplicated asset Do not try to refcount solution syncing when communicating with OOP Delay symbol-search index updating until solution is fully loaded. add more miscellaneous tests for checked operators (dotnet#60727) Support checked operators in explicit interface implementation (dotnet#60715) Avoid formatting diagnostics with raw strings (dotnet#60655) Make heading levels for warning waves documentation consistent (dotnet#60721) Clean up IDiagnosticService extension methods Remove #nullable enable Add integration test to flag MEF composition breaks Generate static abstract interface members correctly (dotnet#60618) Merge release/dev17.2 to main (dotnet#60682) Fix FAR on checked operators (dotnet#60698) Add implement interface support for checked operators and cast operators (dotnet#60719) Update csc.dll path in launch.json (dotnet#60663) Grab bag of UTF8 string support in IDE features (dotnet#60599) Allow code actions to retrieve options for any language (dotnet#60697) Fix flaky VSTypeScriptHandlerTests (dotnet#60706) ...

RikkiGibson · 2022-04-15T17:59:10Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+                        }
+                        else if (initializerKind == (int)SyntaxKind.BaseConstructorInitializer)
+                        {
+                            // If we chained to a `base` constructor, a SetsRequiredMembers attribute applies to the base type's required members only, and the current type's required members


It seems like the code paths which handle SetsRequiredMembers on base but not this could be eliminated.

I'm not sure what you mean by this? Can you elaborate?

I was wrong about this. I recall now that you're still expected to get a different initial state when the 'this()' constructor you chain to has SetsRequiredMembers, versus the 'base()' constructor you chain to having it.

For some reason I expected a further simplification in NullableWalker to fall out of the decision to require SetsRequiredMembers to be used on constructors which chaining to a SetsRequiredMembers constructor.

RikkiGibson

LGTM with one comment which can be addressed in a subsequent PR if you prefer.

333fred requested a review from a team as a code owner March 25, 2022 19:33

dotnet-issue-labeler bot added the Area-Compilers label Mar 25, 2022

333fred requested review from jcouv and RikkiGibson March 25, 2022 19:33

333fred force-pushed the setsrequiredmembers branch from 3d01290 to 7761d6b Compare March 25, 2022 19:41

RikkiGibson self-assigned this Mar 25, 2022

Correct test issues.

a4dd570

Fix EE compile.

a9aef94

runfoapp bot mentioned this pull request Mar 29, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

RikkiGibson reviewed Mar 29, 2022

View reviewed changes

333fred added 3 commits March 29, 2022 14:41

Fix formatting, add additional tests.

95d08c1

Fix used assembly references leg.

d9da29b

PR feedback.

72c3927

RikkiGibson approved these changes Mar 29, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs Outdated Show resolved Hide resolved

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved

Readd condition.

49a53a5

jcouv reviewed Mar 30, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv reviewed Mar 30, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 30, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs Show resolved Hide resolved

jcouv reviewed Mar 30, 2022

View reviewed changes

Use SuppressRequiredMembers to inform nullable analysis of whether pr…

3984820

…operties should be considered initialized.

Add missing member test for SetsRequiredMembers on records.

e69ef15

RikkiGibson reviewed Apr 6, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Apr 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv reviewed Apr 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv requested a review from RikkiGibson April 7, 2022 18:30

333fred added 2 commits April 12, 2022 13:27

Add restriction that SetsRequiredMembersAttribute must be applied whe…

4cfdf57

…n chaining to a constructor with the attribute. Add more tests.

Remove erroneous assert.

e7e4947

jcouv reviewed Apr 14, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv reviewed Apr 15, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

jcouv reviewed Apr 15, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

jcouv reviewed Apr 15, 2022

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Outdated Show resolved Hide resolved

jcouv reviewed Apr 15, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved

jcouv approved these changes Apr 15, 2022

View reviewed changes

jcouv self-assigned this Apr 15, 2022

333fred added 2 commits April 14, 2022 17:41

PR Feedback.

106378e

RikkiGibson reviewed Apr 15, 2022

View reviewed changes

RikkiGibson approved these changes Apr 15, 2022

View reviewed changes

333fred merged commit 53ce8d4 into dotnet:features/required-members Apr 15, 2022

333fred deleted the setsrequiredmembers branch April 15, 2022 22:07

333fred added the New Feature - Required Members Required properties and fields label Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SetsRequiredMembersAttribute #60392

Support SetsRequiredMembersAttribute #60392

333fred commented Mar 25, 2022

333fred commented Mar 25, 2022

333fred commented Mar 28, 2022

RikkiGibson left a comment

333fred commented Mar 29, 2022

RikkiGibson left a comment

333fred commented Mar 29, 2022

jcouv left a comment

jcouv commented Mar 30, 2022 •

edited

Loading

333fred commented Mar 30, 2022

333fred commented Apr 6, 2022

RikkiGibson commented Apr 7, 2022 •

edited

Loading

333fred commented Apr 12, 2022

333fred commented Apr 14, 2022

jcouv left a comment

RikkiGibson Apr 15, 2022

333fred Apr 15, 2022

RikkiGibson Apr 15, 2022

RikkiGibson left a comment

Support SetsRequiredMembersAttribute #60392

Support SetsRequiredMembersAttribute #60392

Conversation

333fred commented Mar 25, 2022

333fred commented Mar 25, 2022

333fred commented Mar 28, 2022

RikkiGibson left a comment

Choose a reason for hiding this comment

333fred commented Mar 29, 2022

RikkiGibson left a comment

Choose a reason for hiding this comment

333fred commented Mar 29, 2022

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Mar 30, 2022 • edited Loading

333fred commented Mar 30, 2022

333fred commented Apr 6, 2022

RikkiGibson commented Apr 7, 2022 • edited Loading

333fred commented Apr 12, 2022

333fred commented Apr 14, 2022

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2022

Choose a reason for hiding this comment

333fred Apr 15, 2022

Choose a reason for hiding this comment

RikkiGibson Apr 15, 2022

Choose a reason for hiding this comment

RikkiGibson left a comment

Choose a reason for hiding this comment

jcouv commented Mar 30, 2022 •

edited

Loading

RikkiGibson commented Apr 7, 2022 •

edited

Loading